Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bugifx ql timeseries units #118

Merged
merged 13 commits into from
May 17, 2024
Merged

Bugifx ql timeseries units #118

merged 13 commits into from
May 17, 2024

Conversation

samaloney
Copy link
Member

@samaloney samaloney commented May 16, 2024 •
edited
Loading

  • Fix units so those in plots match the units in data
  • Fix bug data units did not match the actual data units
    • Data had ct s-1 keV-1 but was really ct keV-1 now data is really in ct s-1 keV-1
  • Reconstruct the QTables from the HDU header pair so get units from headers
  • Update livetime correction to use same function as imaging with the new parameter values
  • Update plot code to use more of sunpy TimeSeries functions
  • Add columns keyword with useful defaults
  • Update tests to check for correct quantities

Closes #104

from matplotlib import pyplot as plt
from sunpy.timeseries import TimeSeries
from stixpy.timeseries.quicklook import HKMaxi, QLBackground, QLLightCurve, QLVariance

ql_lc = TimeSeries(r"https://pub099.cs.technik.fhnw.ch/fits/L1/2020/05/06/QL/solo_L1_stix-ql-lightcurve_20200506_V02.fits")
ql_lc.quantity('4-10 keV').unit
# Unit("ct / (keV s)")

ql_lc.plot()
plt.show()

stix_ql_example

@codecov Codecov
Copy link

codecov bot commented May 16, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.53%. Comparing base (c606ef4) to head (a3aba9d).
Report is 11 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #118      +/-   ##
==========================================
+ Coverage   69.52%   72.53%   +3.00%     
==========================================
  Files          31       31              
  Lines        1887     1857      -30     
==========================================
+ Hits         1312     1347      +35     
+ Misses        575      510      -65

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samaloney
Fix wrong units being set on QL timeseries
6558105 
* Update plot code to use more of sunpy TimeSeries functions
* Add columns keyword with useful defaults
@samaloney
Add change log
cccf11f
@samaloney samaloney requested a review from hayesla May 17, 2024 08:12
DanRyanIrish
DanRyanIrish reviewed May 17, 2024
View reviewed changes
Copy link
Contributor

@DanRyanIrish DanRyanIrish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where's the key change that actually scales the data by time correctly?

stixpy/timeseries/quicklook.py Outdated Show resolved Hide resolved
stixpy/timeseries/quicklook.py Show resolved Hide resolved
samaloney
samaloney commented May 17, 2024
View reviewed changes
stixpy/timeseries/quicklook.py
Comment on lines -538 to +474
data["variance"] = data["variance"].reshape(-1) / (dE * data["timedel"])
data["variance"] = data["variance"].reshape(-1) * u.ct / (delta_energy * data["timedel"].to(u.s))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not totally sure what units should be here TBH or if we should be correcting for live time.

@DanRyanIrish
Copy link
Contributor

OK. Well, as long as the tests pass and the plots look correct, I'm happy to approve this PR.

hayesla
hayesla reviewed May 17, 2024
View reviewed changes
stixpy/timeseries/quicklook.py Outdated Show resolved Hide resolved
stixpy/timeseries/quicklook.py Outdated Show resolved Hide resolved
stixpy/timeseries/quicklook.py Show resolved Hide resolved
stixpy/timeseries/quicklook.py Outdated
r"""
Quicklook X-ray time series from the background detector.

Background monitoring is done in such way that counts from background detector are summed in five specified energy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again I'd say maybe just mention that this is a QL data product and the file

stixpy/timeseries/quicklook.py Outdated Show resolved Hide resolved
hayesla
hayesla approved these changes May 17, 2024
View reviewed changes
Copy link
Contributor

@hayesla hayesla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once tests pass, all good :)

@samaloney
Copy link
Member Author

Urgh - I don't understand the windows test fail on HK it happening at download time which is done in sunpy and the same thing works for the QL 😭

@samaloney samaloney merged commit ba3b71f into TCDSolar:main May 17, 2024
13 checks passed
@samaloney samaloney deleted the bugifx-ql-timeseries-units branch May 17, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hayesla hayesla hayesla approved these changes

@DanRyanIrish DanRyanIrish DanRyanIrish approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Units on timeseries .plot and .quantity are inconsistent
3 participants
@samaloney @DanRyanIrish @hayesla